Btw, this clean up includes simplifying the implementation of paths_matching (previously called paths_matching_unit) by noticing that the result of the computation is just ignored. With all the helpers code around, it was not obvious that the code could be made simpler. Now it is.

Internal benchmarks show consistent improvements in build times (1-2%) and allocations (up to 1%).

Actually, I've just realised that Action_builder.path_matching was just a trivial special case of Action_builder.record, so I nuked it too.

I've removed Action_builder.contents path which can now be replaced with Action_builder.of_memo (Memo.of_thunk_apply Build_system.read_file path), which looks like a little performance improvement in my internal benchmarks.

Action_builder.*file_exists are gone too. They can be trivially implemented via Build_system.file_exists.

I noticed that internally we had a bunch of incorrect uses of Action_builder.memo where we passed a prematurely forced computation, so I expanded the comment for Action_builder.memo.

@snowleopard do you have more changes pending or should I update the rules?

@snowleopard do you have more changes pending or should I update the rules?

There are a couple of more removals I'm working on. I'll ping you in the next couple of days!

@rgrinberg OK, I think I'm done. My next step is changing the implementation of Action_builder.t but that will be done in a separate PR. Please take over from here, and let me know if I messed up anything.

@snowleopard I think you forgot to push Dep.Facts.record_facts and the visibility_override in dune_engine/action.ml

I think you forgot to push Dep.Facts.record_facts and the visibility_override in dune_engine/action.ml

@rgrinberg Oops, thanks. visibility_override is just an internal thing, so I removed it.

I added Dep.Facts.record_facts, though a few other changes happened in dep.ml so I won't be surprised if I'm missing something else... Let me know!

Completed in a series of PR's culminating in #9559